Skip to content

[Woo POS] Fix two failing analytics tests #16083

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 18 commits into from
Sep 5, 2025
Merged

[Woo POS] Fix two failing analytics tests #16083

merged 18 commits into from
Sep 5, 2025
+80 −38

Conversation

@joshheald
Copy link
Contributor

@joshheald joshheald commented Sep 4, 2025 •
edited
Loading

Description

When I run tests locally, sendReceipt_tracks_failure_with_eligible_for_pos_receipt and sendReceipt_tracks_success_with_eligible_for_pos_receipt would always fail on the check for #expect(analyticsProvider.receivedProperties[indexOfEvent]["eligible_for_pos_receipt”] == {value}).

It’s using index 1, because index 0 was an order_created event, which has no properties. That meant that the arrays for properties and events didn’t line up.

When we expected index 1, the tests crashed because of the index being out of range.

I don’t know yet why these aren’t blocking CI.

To fix it, we simply reset the saved events before the When in each test.

I also stopped the order tests from making the cha-ching sound

Steps to reproduce

  1. Run PointOfSaleOrderControllerTests locally
  2. Observe that all tests complete and pass.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@joshheald
Fix two failing analytics tests
1c567f6 
When I run tests locally, `sendReceipt_tracks_failure_with_eligible_for_pos_receipt` and `sendReceipt_tracks_success_with_eligible_for_pos_receipt` would always fail on the check for `#expect(analyticsProvider.receivedProperties[indexOfEvent]["eligible_for_pos_receipt”] == {value})`.

It’s using index 1, because index 0 was an `order_created` event, which has no properties. That meant that the arrays for properties and events didn’t line up.

When we expected index 1, the tests crashed because of the index being out of range.

I don’t know yet why these aren’t blocking CI.

To fix it, we simply reset the saved events before the `When` in each test.
@joshheald joshheald added this to the 23.2 milestone Sep 4, 2025
@joshheald joshheald added type: task An internally driven task. Tests (new) labels Sep 4, 2025
@joshheald joshheald requested a review from staskus September 4, 2025 11:50
@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Sep 4, 2025 •
edited
Loading

App Icon📲 You can test the changes from this Pull Request in WooCommerce iOS Prototype by scanning the QR code below to install the corresponding build.

App NameWooCommerce iOS Prototype
Build Numberpr16083-b1ca363
Version23.2
Bundle IDcom.automattic.alpha.woocommerce
Commitb1ca363
Installation URL43lgphbq3opl8
 Automatticians: You can use our internal self-serve MC tool to give yourself access to those builds if needed.

@joshheald
Copy link
Contributor Author

joshheald commented Sep 4, 2025 •
edited
Loading

Here's some output from CI's unit tests from before the fix, taken from this run:

2025-09-04 12:27:14 BST
[04:27:14]: ▸ Failing tests:
[04:27:14]: ▸ 	AnalyticsTests.startCashPayment_when_invoked_tracks_expected_event()
[04:27:14]: ▸ 	AnalyticsTests.sendReceipt_tracks_success_with_eligible_for_pos_receipt()
[04:27:14]: ▸ 	AnalyticsTests.sendReceipt_tracks_failure_with_eligible_for_pos_receipt()
[04:27:14]: ▸ ** TEST EXECUTE FAILED **

And then a little further down

2025-09-04 12:27:19 BST
Failing tests:
	AnalyticsTests.startCashPayment_when_invoked_tracks_expected_event()
	AnalyticsTests.sendReceipt_tracks_success_with_eligible_for_pos_receipt()
	AnalyticsTests.sendReceipt_tracks_failure_with_eligible_for_pos_receipt()
** TEST EXECUTE FAILED **
[04:27:19]: Exit status: 65
+--------------------------------------------------+
|                   Test Results                   |
+-------------------------+------------------------+
| Number of tests         | 9432 (and 197 retries) |
| Number of tests skipped | 10                     |
| Number of failures      | 0                      |
+-------------------------+------------------------+
[04:27:20]: Test execution failed. Exit status: 65

So it seems like the TEST EXECUTE FAILED wasn't enough to mark the job as failed, because even though it spotted 3 failures, none of them ended up in the count in the table... which is interesting.

@joshheald
Copy link
Contributor Author

joshheald commented Sep 4, 2025

And even with the fix, making the tests pass locally, they still fail on CI. It just doesn't mark the tests as failed.

[05:11:11]: ▸     ✔ collectCashPayment_when_failure_tracks_correct_event() (0.001 seconds)
[05:11:11]: ▸     ⚠️  Test sendReceipt_tracks_success_with_eligible_for_pos_receipt() recorded an issue at PointOfSaleOrderControllerTests.swift:648:13: Expectation failed: (analyticsProvider.receivedProperties[indexOfEvent]["eligible_for_pos_receipt"] as? Bool → false) == true
[05:11:11]: ▸     ✖ sendReceipt_tracks_success_with_eligible_for_pos_receipt() (0.001 seconds) 1 issue(s)
[05:11:11]: ▸     ✔ sendReceipt_without_order_tracks_failure_without_eligible_for_pos_receipt() (0.001 seconds)
[05:11:11]: ▸     ⚠️  Test sendReceipt_tracks_failure_with_eligible_for_pos_receipt() recorded an issue at PointOfSaleOrderControllerTests.swift:695:17: Expectation failed: (analyticsProvider.receivedProperties[indexOfEvent]["eligible_for_pos_receipt"] as? Bool → false) == true
[05:11:11]: ▸     ✖ sendReceipt_tracks_failure_with_eligible_for_pos_receipt() (0.003 seconds) 1 issue(s)
[...]
[05:13:28]: ▸ Failing tests:
[05:13:28]: ▸ 	AnalyticsTests.startCashPayment_when_invoked_tracks_expected_event()
[05:13:28]: ▸ 	AnalyticsTests.sendReceipt_tracks_success_with_eligible_for_pos_receipt()
[05:13:28]: ▸ 	AnalyticsTests.sendReceipt_tracks_failure_with_eligible_for_pos_receipt()
[05:13:28]: ▸ 	AlamofireNetworkTests.test_concurrent_requests_fail_with_sessionDeinitialized_error_when_ensuresSessionManagerIsInitialized_is_false()
[05:13:28]: ▸ ** TEST EXECUTE FAILED **

@joshheald joshheald requested a review from jaclync September 4, 2025 17:30
@joshheald
Copy link
Contributor Author

joshheald commented Sep 4, 2025

Only one review needed. Nothe that this comes before #16088 – that's where I've fixed the CI reporting of the tests.

@joshheald joshheald mentioned this pull request Sep 4, 2025
Merged
1 task
@wpmobilebot wpmobilebot modified the milestones: 23.2, 23.3 Sep 5, 2025
@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Sep 5, 2025

Version 23.2 has now entered code-freeze, so the milestone of this PR has been updated to 23.3.

@jaclync jaclync self-assigned this Sep 5, 2025
jaclync
jaclync approved these changes Sep 5, 2025
View reviewed changes
Copy link
Contributor

@jaclync jaclync left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking a look and disabling the cha-ching sound! Feel free to merge after checking if CI passes after the suggested changes.

@joshheald joshheald enabled auto-merge September 5, 2025 09:36
@joshheald joshheald merged commit f374495 into trunk Sep 5, 2025
14 checks passed
@joshheald joshheald deleted the task/fix-receipt-eligibility-analytics-test branch September 5, 2025 09:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jaclync jaclync jaclync approved these changes

@staskus staskus Awaiting requested review from staskus

Assignees

@jaclync jaclync

Labels

Tests (new) type: task An internally driven task.

Projects

None yet

Milestone

23.3

Development

Successfully merging this pull request may close these issues.

4 participants

@joshheald @wpmobilebot @jaclync